Conversation
- Add getEddsaSignatureShareRound{1,2,3} and verifyBitGoEddsaMessageRound{1,2}
helpers in sdk-core for building/verifying PGP-signed MPS broadcast messages
- Parameterise partyId/otherSignerPartyId (defaults: user=0, bitgo=2) to
support non-user signers without hardcoding
- Wire eddsaMpcV2 type in sendSignatureShareV2 (common.ts)
- Export helpers as EddsaMPCv2Utils from sdk-core public index
- Add generateEdDsaDKGKeyShares to sdk-lib-mpc MPSUtil (mirrors DklsUtils for ECDSA)
- Add unit tests covering all 5 helpers using io-ts decodeWithCodec for assertions
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TICKET: WCI-153
zahin-mohammad
left a comment
zahin-mohammad
left a comment
There was a problem hiding this comment.
Built and tested locally — tsc --noEmit is clean in both sdk-lib-mpc and sdk-core, and the new sdk-core EdDSA MPCv2 test suite passes (6/6). Confirmed #8697 is the follow-up that wires EddsaMPCv2Utils.signRequestBase into these helpers.
One semantics concern that I think is worth resolving before merge because it freezes on the public surface — MPSUtil.generateEdDsaDKGKeyShares is barrel-exported, so the seed contract becomes part of the SDK API. The rest are follow-up-safe: helper renames (still internal until #8697 lands deep-import callers), test-util consolidation (pure refactor), and a few test/cleanup nits.
See inline comments.
| const privKey = seed ? seed.subarray(0, 32) : crypto.randomBytes(32); | ||
| const pubKey = Buffer.from(x25519.getPublicKey(privKey)); | ||
| return { privKey: Buffer.from(privKey), pubKey }; | ||
| } |
There was a problem hiding this comment.
Seed dual-use — please address before merge. The same seed bytes are used here as the X25519 private key and below at L45-47 as the DKG round-0 seed (getFirstMessage(seedX)). The original test-util version of this function had a comment that explicitly flagged this ("Seeds are used as X25519 private keys AND as DKG round0 seeds for full determinism"); promoting to production source dropped the warning.
Since MPSUtil.generateEdDsaDKGKeyShares is exported via the package barrel, this is now public API. Either restore the explicit "AND" comment so callers know what they're opting into, or split inputs (e.g. accept encKey/dkgSeed per party, or HKDF-derive both from one seed). I'd lean toward splitting — once callers depend on the current deterministic mapping, changing it is a breaking change.
Also, no length check: seed.subarray(0, 32) on a < 32-byte buffer produces a shorter view, then x25519.getPublicKey throws invalid scalar, expected 32 or 32 bytes, got N. A friendlier assert(!seed || seed.length >= 32, ...) up-front would surface the error at the right layer.
| * | ||
| * Mirrors `DklsUtils.generateDKGKeyShares` for ECDSA DKLS. | ||
| */ | ||
| export async function generateEdDsaDKGKeyShares( |
There was a problem hiding this comment.
Duplicate of the existing test helper. This function body is near-verbatim from modules/sdk-lib-mpc/test/unit/tss/eddsa/util.ts (only comments changed). The pre-existing test/unit/tss/eddsa/dkg.ts:5 and dsg.ts:4 still import { generateEdDsaDKGKeyShares } from './util', so they keep using the test-util copy — the two implementations will silently diverge.
Suggested fix (internal-only, follow-up safe): replace the body of the test util with a re-export so existing tests still resolve via ./util:
export { generateEdDsaDKGKeyShares } from '../../../src/tss/eddsa-mps/util';Leave runEdDsaDSG in the test util — it's test-only orchestration.
| * PGP-signs the WASM round-0 broadcast message with the signer's ephemeral key and | ||
| * wraps it into a SignatureShareRecord ready for `sendSignatureShareV2`. | ||
| */ | ||
| export async function getEddsaSignatureShareRound1( |
There was a problem hiding this comment.
Naming inconsistent with ECDSA equivalents. Compare modules/sdk-core/src/bitgo/tss/ecdsa/ecdsaMPCv2.ts:30 — getSignatureShareRoundOne/Two/Three (English numerals, no Ecdsa prefix). Here we have getEddsaSignatureShareRound1/2/3 (digits + redundant prefix since the file already lives under tss/eddsa/). Same for verifyBitGoEddsaMessageRound1/2 vs ECDSA's verifyBitGoMessagesAndSignaturesRoundOne/Two.
Follow-up-safe today (these aren't barrel-exported), but #8697 adds deep-import callers — easier to pick names now than to ripple a rename later. Suggest getSignatureShareRoundOne/Two/Three + verifyBitGoMessageRoundOne/Two.
| */ | ||
| export async function verifyBitGoEddsaMessageRound1( | ||
| parsedRound1Output: EddsaMPCv2SignatureShareRound1Output, | ||
| bitgoGpgKey: openpgp.Key, |
There was a problem hiding this comment.
Nit: parameter is named bitgoGpgKey but the function accepts any peerPartyId: 0 | 1 | 2 — when called with USER or BACKUP as the peer, the name lies. peerGpgKey would match the contract. Same comment applies to verifyBitGoEddsaMessageRound2 at L93. Follow-up safe.
| ): Promise<MPSTypes.DeserializedMessage> { | ||
| const rawBytes = await MPSComms.verifyMpsMessage(parsedRound1Output.data.msg1, bitgoGpgKey); | ||
| return { | ||
| from: peerPartyId as MPCv2PartiesEnum, |
There was a problem hiding this comment.
Nit: peerPartyId as MPCv2PartiesEnum is an unsafe cast that's only needed because the parameter type is the literal union 0 | 1 | 2. Typing the parameter as MPCv2PartiesEnum directly removes the cast and makes the contract clearer at the call site. Follow-up safe.
| data: { msg2: bitgoSignedMsg2 }, | ||
| }; | ||
|
|
||
| void bitgoSignedMsg1; |
There was a problem hiding this comment.
Nit: looks like leftover dead code from a refactor — bitgoSignedMsg1 is computed above and then explicitly discarded with void. Safe to remove both the assignment and this line. Follow-up safe.
| assert.ok(parsed.data.msg3.message, 'msg3.message should be set'); | ||
| assert.ok(parsed.data.msg3.signature, 'msg3.signature should be set'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test coverage gaps worth filing as follow-ups (none blocking):
- Round-2 verify has no tampered-signature negative test (round 1 has one at L94-108)
- BACKUP path (
partyId=1) ofgetEddsaSignatureShareRound{1,2,3}is never exercised - Deterministic-seed branch of
generateEdDsaDKGKeySharesis untested — the test calls it without seeds, so that code path isn't verified - No direct assertion that all 3 DKG parties agree on the public key (currently only used as setup)
2 participants
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
TICKET: WCI-153